feat: deprecate OpenAI config in favor of LiteLLM agyn key#1263
feat: deprecate OpenAI config in favor of LiteLLM agyn key#1263casey-brooks wants to merge 3 commits intomainfrom
Conversation
|
Test & Lint Summary:
|
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for the refactor—I've left a few blockers that showed up during the chunked pass:
- The new git-based resolver no longer uses our configured GitHub credentials, so private flakes regress to
unauthorized_private_repo. - Annotated tag resolution now returns the tag object SHA instead of the peeled commit, which breaks
flake.nixfetches. - Aborted
ls-remotecalls bubble up as genericGitCommandError, so clients stop seeing the documented 504 timeout.
Once those are addressed I'm happy to take another look.
| } | ||
|
|
||
| private buildGitRemote(repo: NormalizedRepository): string { | ||
| return `https://github.com/${repo.owner}/${repo.repo}.git`; |
There was a problem hiding this comment.
[major] We no longer surface the configured GitHub credentials after this refactor. always returns the public HTTPS URL, and drops the Authorization header entirely. In we relied on here so private Nix flakes could be resolved. With the new code every private repo (or any repo that requires auth via PAT/GitHub App) falls straight into the branch even when the operator has provided credentials. Please plumb the configured token/app identity through both the git remote selection and the raw fetch so that the existing private-repo support keeps working.}
| return null; | ||
| } | ||
| for (const line of lines) { | ||
| const [sha] = line.split(/\s+/); |
There was a problem hiding this comment.
[major] Annotated tags regress here. returns two lines: one for the tag object () and one for the peeled commit (). Because we return the first 40-hex entry, the code now surfaces the tag object SHA. Later we feed that value into , which resolves to a 404 because GitHub expects the commit/tree SHA, not the tag object. The previous GitHub API call dereferenced tags automatically. Please make sure we prefer the peeled entry (or otherwise dereference tags) before returning so annotated tags keep working.}
| signal, | ||
| }); | ||
| if (res.status === 404) { | ||
| throw new FetchErrorResponse(409, { error: 'non_flake_repo' }); |
There was a problem hiding this comment.
[major] When the timeout fires we abort the shared . will reject with an , but this wrapper converts every failure into . Upstream we now lose the ability to detect the abort, so the catch block at the top falls through to a 500 instead of the documented 504 . Please preserve the native (or at least surface a sentinel we can detect) so the client still sees the timeout semantics we had before.}
|
Addressed @noa-lucent's feedback:
Local verification:
|
|
Updated resolve-repo per public-only directive:
Tests:
|
rowan-stein
left a comment
There was a problem hiding this comment.
Approved (Noa: no blockers; minor suggestion noted).
noa-lucent
left a comment
There was a problem hiding this comment.
Thanks for the updates—annotated tag resolution and abort propagation look good now. The remaining gap is authenticated access: both the ls-remote calls and the raw flake.nix fetch always hit GitHub anonymously, so private flakes still regress to unauthorized_private_repo. Please plumb the configured App/PAT token through to git and to the raw fetch (restoring the behaviour provided by the old controller) so private repo support keeps working.
| } | ||
|
|
||
| private buildGitRemote(repo: NormalizedRepository): string { | ||
| return `https://github.com/${repo.owner}/${repo.repo}.git`; |
There was a problem hiding this comment.
[major] The git remote we pass to ls-remote still drops the configured GitHub credentials. With the previous controller we surfaced either the GitHub App token or the operator-provided PAT via the Authorization header, so private flakes resolved cleanly. In the new flow we always call https://github.com/<owner>/<repo>.git without credentials, and ls-remote now fails with authentication required for any private repo. Please plumb the configured identity (App installation token, PAT, etc.) into the remote—e.g. https://x-access-token:<token>@github.com/…—so we keep the existing private-repo support.}
|
|
||
| private async ensureFlakePresent(repo: NormalizedRepository, commitSha: string, signal: AbortSignal): Promise<void> { | ||
| const rawUrl = `https://raw.githubusercontent.com/${repo.owner}/${repo.repo}/${commitSha}/flake.nix`; | ||
| const res = await this.fetchImpl(rawUrl, { |
There was a problem hiding this comment.
[major] Likewise the raw fetch for flake.nix no longer carries our GitHub credentials. Private repositories respond with 404/401/403 here even when the operator configured a PAT/App, so every private flake now returns unauthorized_private_repo. The previous implementation delegated to githubRequest, which attached the Authorization header. Please restore authenticated access (reuse the App/PAT token you surface to git) before we can ship this.}
Summary
Testing